Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A new task to generate labels #1443

Merged
merged 1 commit into from
Oct 11, 2024
Merged

A new task to generate labels #1443

merged 1 commit into from
Oct 11, 2024

Conversation

ralphbean
Copy link
Member

For https://issues.redhat.com/browse/KONFLUX-4274. Turns out people really do use those labels (see linked issues).

This can have a future connection with #1268 once we do that. I.e., use the commit timestamp for the release label and build-date, rather than the real world date. But, that's for another day...

@ralphbean ralphbean requested review from chmeliik and arewm and removed request for MartinBasti and mkosiarc September 18, 2024 20:29
@ralphbean ralphbean force-pushed the release-label branch 3 times, most recently from 90a25b5 to 62e7614 Compare September 25, 2024 00:18
@ralphbean ralphbean changed the title Optionally set the release label to the build timestamp A new task to generate labels Sep 25, 2024
task/generate-labels/0.1/generate-labels.yaml Outdated Show resolved Hide resolved
task/generate-labels/0.1/generate-labels.yaml Outdated Show resolved Hide resolved
task/generate-labels/0.1/generate-labels.yaml Outdated Show resolved Hide resolved
@ralphbean ralphbean force-pushed the release-label branch 2 times, most recently from 897f212 to 7d5587b Compare September 26, 2024 12:30
@ralphbean ralphbean requested review from arewm and chmeliik September 28, 2024 13:57
@arewm
Copy link
Member

arewm commented Oct 1, 2024

There has been more interest in this type of functionality from users (i.e. to set/forward the git tags so that they can be used at release time). Should we add this to the default pipelines?

@mmorhun , have there been any criteria defined for when to add tasks to the default pipelines?

@samdoran
Copy link
Contributor

samdoran commented Oct 1, 2024

I would love the ability to pass in a list of key=value labels to the build tasks. That wolud save some steps compared to what I am doing today.

I'm not as clear on the value of the generate-labels task. It seems limited to set of four values that can be applied to arbitrary keys.

What would provide more utility and flexibility is a task parameter for an executable in the repository that returns a JSON list of key=value pairs.

@arewm
Copy link
Member

arewm commented Oct 1, 2024

I'm not as clear on the value of the generate-labels task. It seems limited to set of four values that can be applied to arbitrary keys.

The specific values mentioned are just those that it helps to interpolate. I think that it should be possible to modify this task parameters to add any variables that you want. For example, would something like this work?

   - name: label-templates
     value: 
        - "release=$SOURCE_DATE_EPOCH"
        - "build-date=$SOURCE_DATE"
        - "short-commit=$(tasks.git-clone.results.short-commit)"
   - name: source-date-epoch
     value: '$(tasks.git-clone.results.commit-timestamp)

(well, commit-timestamp might actually provide a pretty date, not an epoch ... )

This task can be used to inject content into labels (from other tasks' results, for example). This does provide some indirection ... but if you don't need any of the label templates then you can also use the new parameter in the buildah task to directly add desired labels without having to use an ARG-based indirection.

@samdoran
Copy link
Contributor

samdoran commented Oct 1, 2024

It seems like the ADDITIONAL_LABELS parameter is sufficient for our needs. I don't see an immediate use for the label templates task, but that's just for our projects.

@chmeliik
Copy link
Contributor

chmeliik commented Oct 7, 2024

What would provide more utility and flexibility is a task parameter for an executable in the repository that returns a JSON list of key=value pairs.

This is tricky, because executing arbitrary code before the build voids the "trust" of the build pipeline.

It would be doable in a trusted-artifacts pipeline - it would have to be a task that takes the source-code artifact as input but doesn't return a source-code artifact as output (any potential modifications would be local-only). And rather than running an executable from the source repo, the task could just execute any arbitrary script passed in (the script would run with the source repo as PWD, so could execute anything in the repo if needed).

Maybe we should just add copy-and-pasteable snippets to the docs showing how to insert custom tasks into a trusted-artifacts pipeline in the least painful way

@ralphbean
Copy link
Member Author

Yeah, perhaps if we ran that arbitrary executable with unshare and no net, that would be acceptable. But, in any case - I think it's out of scope of this change.

@samdoran
Copy link
Contributor

samdoran commented Oct 7, 2024

This is tricky, because executing arbitrary code before the build voids the "trust" of the build pipeline. It would be doable in a trusted-artifacts pipeline...

I figured there would be some challenges with running arbitrary code. I'm doing this today in a custom task with a trusted artifact and it meets our needs while not compromising the integrity of the build artifact.

Perhaps this is something best left to individual pipeline authors to implement as needed.

Maybe we should just add copy-and-pasteable snippets to the docs showing how to insert custom tasks into a trusted-artifacts pipeline in the least painful way.

That's probably the best solution. It took me way more time than I would like to admit to get something so "simple" working, especially with the trusted artifacts model.

Copy link
Member

@arewm arewm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks reasonable to me. Thanks.

Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple minor improvements, otherwise LGTM

task/buildah/0.2/buildah.yaml Outdated Show resolved Hide resolved
task/buildah/0.2/buildah.yaml Outdated Show resolved Hide resolved
task/buildah/0.2/buildah.yaml Outdated Show resolved Hide resolved
task/generate-labels/0.1/README.md Outdated Show resolved Hide resolved
task/generate-labels/0.1/generate-labels.yaml Outdated Show resolved Hide resolved
task/generate-labels/0.1/generate-labels.yaml Outdated Show resolved Hide resolved
@ralphbean ralphbean force-pushed the release-label branch 4 times, most recently from 8e093b5 to 65e78a1 Compare October 9, 2024 01:23
@ralphbean ralphbean force-pushed the release-label branch 2 times, most recently from b978eda to 6a5cae9 Compare October 9, 2024 20:07
build_args+=("$@")

LABELS=()
# Split `args` into two sets of arguments.
Copy link
Collaborator

@mmorhun mmorhun Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks complicated.
Could we have additional labels in a dedicated env var for example?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started that way @mmorhun, but if we make the labels available to the script as an env var, then I don't think I can make the param an array.

Copy link
Collaborator

@mmorhun mmorhun Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will a delimiter in the env var work? Like label1=value1|label2=something else

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Label values are too free-form for delimiters to work

Ralph's original approach was to use a JSON string of an array, but that's much less user-friendly

You can't really make something like this work with a JSON string

params:
  - name: LABELS
    value:
      - $(tasks.generate-labels.results.LABELS[*])
      - foo=bar

@ralphbean ralphbean requested a review from chmeliik October 10, 2024 17:09
shift
while [[ $# -gt 0 && $1 != --* ]]; do
LABELS+=("--label" "$1")
shift
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's just the TA task generation doing some formatting ¯\_(ツ)_/¯

Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, please squash into a number of commits that you consider appropriate before merging

@ralphbean ralphbean enabled auto-merge October 11, 2024 17:30
@ralphbean ralphbean added this pull request to the merge queue Oct 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 11, 2024
@arewm arewm added this pull request to the merge queue Oct 11, 2024
Merged via the queue into main with commit 44a6463 Oct 11, 2024
14 checks passed
@ralphbean ralphbean deleted the release-label branch October 12, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants